-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup #1
Cleanup #1
Conversation
Neat, thanks for the PR! I'll take a closer look at this soon, but do have some initial comments:
Is there anything explicitly 2021 here or would 2018 suffice? I'm not strictly opposed to 2021, but it might be worth relaxing the MSRV if there's no need to.
Why not 0.5 (and from what I can tell, a 1.0 is about to be released)?
I haven't been keeping up with the ecosystem, what is the rationale here? this issue in particular caught my eye, and is a dealbreaker if still true.
I was confused by this at first, but I'm guessing it has something to do with appeasing a rustc warning around packed struct accesses? I highly doubt there's UB or unaligned accesses happening there unless the whole container struct is unaligned - and if it is, this change seems unlikely to actually fix it (if widestring's
With winapi part of the public api that makes this a breaking change to It might be worth splitting this into multiple PRs to make it easier to discuss/review/land? |
Hi there, sorry about closing this with no response. I think I inadvertently closed it when merging this in https://github.com/AstroHQ/ddc-winapi-rs.
2018 is enough.
I will updated it to latest in a new PR.
I haven't tried cross-compiling from Linux. Mostly the switch was motivated by our good experience with the better ergonomics of the windows-rs bindings (
AFAIK just creating a reference to a field of a packed struct is UB (https://doc.rust-lang.org/nightly/std/ptr/fn.read_unaligned.html#on-packed-structs). However you make a good point about the widestring fn also requiring the ptr to have proper alignment. I can make a new PR where I copy the data and pass that copy to widestring. I guess that would fix all of it? Thanks for the reply!, I'll open new shorter PRs probably the next week :) |
btw, I just noticed recently I opened a new one with basically the same content. Sorry for the mess, I'll close that one too 😅. |
No problem! I saw the new PR but hadn't had a chance to go over it yet. I plan on pulling in most of these changes anyway, it was just a bit hard to review all at once.
I recently set up cross compiles as part of ddcset's CI, but haven't tested it with the
It's a bit less absolute IIRC - it's only UB if the reference created is actually unaligned for the referenced type. And since
A proper fix would actually be to just ensure that it's aligned. I feel like the fact that the structure is marked "packed" may be a bug to begin with (or maybe that's just windows.h convention?), but... it's easy enough to just make |
winapi
->windows